[pickers] Support onMonthChange / onYearChange in day view
#6954
Conversation
flaviendelangle
added
bug 🐛
These are the results for the performance tests:
|
| @@ -473,7 +476,7 @@ export const DateCalendar = React.forwardRef(function DateCalendar<TDate>( | |||
| openView={openView} | |||
| currentMonth={calendarState.currentMonth} | |||
| onViewChange={setOpenView} | |||
| onMonthChange={(newMonth, direction) => handleChangeMonth({ newMonth, direction })} | |||
| onMonthChange={slideToMonth} | |||
There was a problem hiding this comment.
I now compute the direction inside useCalendarState to have a single month change method exposed
| @@ -71,18 +72,27 @@ export const createCalendarStateReducer = | |||
| !disableSwitchToMonthOnDayFocus && | |||
| !utils.isSameMonth(state.currentMonth, action.focusedDay); | |||
|
|
|||
| let slideDirection: SlideDirection; | |||
| if (!needMonthSwitch) { | |||
| slideDirection = state.slideDirection; | |||
There was a problem hiding this comment.
Before, the slide to the new month was caused by the useEffect tracking the external value.
So the focus of the clicked day was already done
But now it is the click itself which caused the slide (like when you click on the array left / right).
Without this check, the focus was resetting the slide direction, causing it to go on the wrong direction in some scenario.
| type: 'changeMonth', | ||
| ...payload, | ||
| }); | ||
| const slideToMonth = useEventCallback((newDate: TDate) => { |
There was a problem hiding this comment.
Renamed to make clear that we are not updating any value, just "meta data" used to handle the visually displayed month
There was a problem hiding this comment.
If we are going this route, then maybe onYearChange should also be called when year changes when navigating months (not only when choosing a date in a different month—which is also in a different year)? 🤔
| setValue(newValue); | ||
| onChange?.(newValue, selectionState); | ||
|
|
||
| if (newValue != null) { |
There was a problem hiding this comment.
We now do the checks here to make sure that any value change causes a call to onMonthChange /onYearChange if needed
flaviendelangle
changed the title
onMonthChange when clicking on a day from another month
flaviendelangle
changed the title
onMonthChange when clicking on a day from another monthonMonthChange / onYearChange in day view
There was a problem hiding this comment.
The implementation sounds good to be a fix on master. For next I'm wondering if we should rename onMonthChange.
When I see onMonthChange I expect it to get called when the selected value moves from one month to an other. But it's also called when I press Next/Previous month buttons.
So maybe onVisibleMonthChange would be more appropriate
Good point |
There was a problem hiding this comment.
IMO—this implementation seems incorrect.
Even the documentation states that onYearChange and onMonthChange are callbacks called when the year or month changes, but now onMonthChange is called when navigating months—which contradicts the docs.
I'm with Alex on this one.
If we want to emit year or month navigation changes—they ought to be separate callbacks in order to avoid possible head-aches.
| const selectNextMonth = React.useCallback(() => { | ||
| changeMonth(utils.getNextMonth(currentMonth)); | ||
| }, [changeMonth, currentMonth, utils]); | ||
| slideToMonth(utils.getNextMonth(currentMonth)); | ||
| }, [slideToMonth, currentMonth, utils]); |
There was a problem hiding this comment.
Nitpick: Should we maybe replace these with useEventCallback? 🤔
| type: 'changeMonth', | ||
| ...payload, | ||
| }); | ||
| const slideToMonth = useEventCallback((newDate: TDate) => { |
There was a problem hiding this comment.
If we are going this route, then maybe onYearChange should also be called when year changes when navigating months (not only when choosing a date in a different month—which is also in a different year)? 🤔
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
added
the
PR: out-of-date
Fixes #6953